Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User feed page refactor #1144

Merged
merged 12 commits into from
Mar 5, 2024
Merged

User feed page refactor #1144

merged 12 commits into from
Mar 5, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Feb 21, 2024

Pull Request Description

This is the second attempt at refactoring the user feed page. This time, I've decided to limit down the scope of implementation to be concentrated on the feed (rather than both the feed and account pages). Because of this, there are some files which I've renamed (as they'll be deprecated in the future) to allow the account page to use the previous logic.

Caveats:

  • Comment actions are not implemented yet. This means that swipe actions, or any action on a comment will not be triggered at this time.
  • Some FAB actions don't make sense when in the user page/comments tab (create post, subscriptions (as a new page is pushed), dismiss read)
  • When triggering the user sidebar, it looks a bit weird since the posts/comments section hides itself (otherwise, the sidebar would show under the posts/comments tab bar)

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member

micahmo commented Feb 22, 2024

Good luck! 😊😊

@hjiangsu
Copy link
Member Author

@micahmo I think this is functional enough for you to try it out! I sure I missed something during my implementation so let me know if you notice any weird behaviours. I mentioned some caveats I found while testing this myself (will most likely attempt to fix the first and second bullet points in separate PRs later)

Also, no need to do any code reviews or anything since there's been a lot of changes 😅

@Fmstrat
Copy link
Contributor

Fmstrat commented Feb 24, 2024

Just because of time I wish I had but don't, I would highly recommend merging the read-on-scroll branch before this one, it looks like there will be conflicts here, and you'll be much more suited to understanding them than me.

@hjiangsu
Copy link
Member Author

I agree! I'll be merging in all other PRs before I merge this one in to minimize as many conflicts as possible from your end 😄

This is still in draft too since I feel like there's more testing to be done

@micahmo
Copy link
Member

micahmo commented Feb 24, 2024

First of all, nice work tackling one of the big refactoring tasks we know we have to do! Per your recommendation, I did not review the code 😊 I just played around with things a bit.

I did notice a couple funny things about the header and sidebar. It looks like the header is now word wrapping, which looks funny when there's no natural breakpoint. It also looks like there's some render overflow on the sidebar, despite there being no content down there. (Note that this is only on my user profile page, but I think that does include your sidebar changes.)

image

The toggle button hiding is a bit funny, but at least it's animated, so we can live with that!

I was wondering how comment sorting works since some of the modes don't apply (at least to comments within a post).

That's all for now, nothing too crazy!

@hjiangsu
Copy link
Member Author

It looks like the header is now word wrapping, which looks funny when there's no natural breakpoint.

Interesting! I copied the behaviour of the community header, so that might have something to do with it. Maybe I can change it here so that it scales the text down based on the length of the name.

It also looks like there's some render overflow on the sidebar, despite there being no content down there.

Hmm, I'll take a look at that!

The toggle button hiding is a bit funny, but at least it's animated, so we can live with that!

Yeah, I wasn't sure of a good way to do it without overcomplicating the logic. This is because the tabs are not technically a part of the header, so when the sidebar shows up, it shows up under the tab bar (which I think looks more awkward to say the least) If you have any suggestions, let me know!

I was wondering how comment sorting works since some of the modes don't apply (at least to comments within a post).

The endpoint that we use to retrieve the user posts/comments are the same, and it seems like it takes in the sorting of the post 😅 I think the Lemmy backend handles this some way but I'm not too sure myself

@hjiangsu
Copy link
Member Author

Alright, I've updated this a bit to fix some of the issues:

  • The username should now fit in one line. This does mean that the font size is now variable, but hopefully it shouldn't be too big of a deal!
  • Added back the user share option (missed it when I was re-introducing the user app bar) and also moved some options to the overflow menu to (to match more with the community app bar) - more specifically, moved the refresh/share into the overflow menu. Let me know what you think!

It also looks like there's some render overflow on the sidebar, despite there being no content down there.

This might be due to the SizedBox that is at the end of the sidebar to allow it to scroll up more. I don't think this affects functionality per-say, so I left it as is.

@micahmo
Copy link
Member

micahmo commented Feb 27, 2024

  • The username should now fit in one line. This does mean that the font size is now variable, but hopefully it shouldn't be too big of a deal!

Oh yeah, that looks awesome!
I wonder about the full name on the line below. In my screenshot, it was able to break at the bullet, but what if the name were so long that it had to break in the middle of the text? Should we just overflow will ellipses there, or apply the same fix with the font scaling? Also this would be sweet in the community header as well, but that can be later. 😊

Otherwise I think this is good to go!

@hjiangsu
Copy link
Member Author

I wonder about the full name on the line below. In my screenshot, it was able to break at the bullet, but what if the name were so long that it had to break in the middle of the text? Should we just overflow will ellipses there, or apply the same fix with the font scaling?

I was a bit torn on this. I tried to make the full name fit in one line, but that made it difficult to read for some long usernames. Using ellipses makes it so that we can't fully read the name which is also something I'd like to avoid. I think we can leave it as is for now and see if this is an issue?

Otherwise I think this is good to go!

Nice! I'll wait until I can merge in some other PRs first (e.g., mark as read on scroll) and then merge this in!

@hjiangsu hjiangsu marked this pull request as ready for review February 29, 2024 02:34
@hjiangsu hjiangsu changed the title Draft: User feed page refactor User feed page refactor Feb 29, 2024
@micahmo
Copy link
Member

micahmo commented Feb 29, 2024

I think we can leave it as is for now and see if this is an issue?

Sounds good! By the way, I compared against the current behavior. It looks like the short username is truncated with a fade out and the full name is truncated with ellipses. Wrapping is fine for now; maybe we can think of something more elegant in the future. 😊

@hjiangsu
Copy link
Member Author

hjiangsu commented Mar 5, 2024

I'll go ahead and merge this in! Let me know if you find any additional issues while running with this change 😄

@hjiangsu hjiangsu merged commit 02367b7 into develop Mar 5, 2024
1 check passed
@hjiangsu hjiangsu deleted the refactor/user-page branch March 5, 2024 16:29
@micahmo
Copy link
Member

micahmo commented Mar 5, 2024

Woohoo!

@micahmo
Copy link
Member

micahmo commented Mar 6, 2024

Just FYI, it looks like the full username wrapping is not quite right with a really long name (ThunderDevTesting • sh.itjust.works)

image image

@micahmo
Copy link
Member

micahmo commented Mar 13, 2024

Just FYI, in addition to the above, I found another different behavior on the current user page vs. other pages. The former has no shadow over the sidebar while the latter does.

Header Header
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants